fix(0.6.0): establish safe persistence failure boundaries - #225
Conversation
Persistence stores now expose fixed cause-free failure text; raw paths, SQL text, persisted payloads, and arbitrary exception messages flow only to an explicitly configured PersistenceFailureDiagnosticObserver. - Add PersistenceFailureCode / PersistenceResourceKind / PersistenceOperation, PersistenceFailureDiagnosticObserver + event, and WorkflowPersistenceFailureException. - Add persistenceBoundary() with cancellation-first execution, fail-open observer delivery, trusted safe-factory pass-through, and re-sanitization of untrusted caller-constructed domain exceptions. - Wrap checkpoint (file/markdown/JDBC/in-memory), lease (file/JDBC/in-memory), step-attempt (file/JDBC/in-memory) and worker-registry operations; corrupt payload/path/field values ride only on internal Corrupt*Exception carriers to the observer. - Preserve exception and store JVM descriptors: class-body failureCode and safeFactoryTrusted props with internal setters, additive observer constructor overloads, existing primary constructors unchanged (api dump + v0.5.0 binary fixture extended to 17 markers). - Worker observers receive safe terminal failures; LoggingTramaiWorkerObserver renders exception class names only. - Add PersistenceSafeFailureBoundaryTest (14 adversarial leak scenarios) and update contract tests to the fixed-text public contract without weakening cancellation, CAS, durability, or recovery assertions. - Mark Epic 1.2 Safe Error Boundaries complete in roadmap, characterization matrix, safe-error-boundaries concept, orchestration-persistence guide, and changelog.
There was a problem hiding this comment.
Pull request overview
Establishes a safe persistence failure boundary across all built-in persistence implementations in tramai-orchestration, ensuring filesystem paths, SQL text, persisted payloads, and arbitrary exception messages do not cross into caller-visible exceptions, ordinary worker observers, or default logs. The original failure is retained only via an explicitly configured PersistenceFailureDiagnosticObserver, aligning persistence with the previously-merged safe-boundary slices for tools, providers, workflow steps, and structured output.
Changes:
- Adds a typed persistence failure model (
PersistenceFailureCode,PersistenceResourceKind,PersistenceOperation) plus a single cancellation-firstpersistenceBoundary(...)implementation and safe fixed-message factories. - Wires the boundary into all built-in checkpoint / lease / step-attempt / worker-registry persistence families, and ensures worker observer/logging paths receive only safe failures.
- Extends binary-compat fixture coverage and adds adversarial leak tests + doc/ADR-style updates describing the persistence boundary contract.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/PersistenceFailures.kt | New central persistence safe-boundary implementation, typed codes, diagnostic observer, and fixed-message safe factories. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/WorkflowPersistence.kt | Updates checkpoint APIs/exceptions and in-memory store to use the new persistence boundary + safe failures. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/WorkflowLease.kt | Updates lease exceptions and in-memory lease/worker-registry operations to route through the persistence boundary. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/FileWorkflowCheckpointStore.kt | Wraps file-backed checkpoint operations in the safe boundary; corruption routes via internal carriers. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/MarkdownWorkflowCheckpointStore.kt | Wraps markdown checkpoint operations in the safe boundary; fail-closed corruption handling via internal carrier. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/JdbcWorkflowCheckpointStore.kt | Wraps JDBC checkpoint operations in the safe boundary; fail-closed corruption handling during row decoding. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/FileWorkflowLeaseStore.kt | Wraps file-backed lease operations in the safe boundary; fixed-text fencing/stale lease failures. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/JdbcWorkflowLeaseStore.kt | Wraps JDBC lease operations in the safe boundary; replaces detailed conflicts with safe fixed-text failures. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/FileStepAttemptRecordStore.kt | Wraps file-backed step-attempt operations in the safe boundary; corruption uses internal carrier exceptions. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/JdbcStepAttemptRecordStore.kt | Wraps JDBC step-attempt operations in the safe boundary; corruption uses internal carrier exceptions. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/StepAttemptRecord.kt | Extends step-attempt corruption exception with safe-factory trust metadata. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/StepAttemptRecordCodec.kt | Converts codec failures to internal corruption carriers to avoid leaking raw values in public exceptions. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/TramaiWorker.kt | Sanitizes persistence failures before sending them to ordinary worker observers. |
| tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/LoggingTramaiWorkerObserver.kt | Stops logging Throwable.message; logs exception class names only. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/PersistenceSafeFailureBoundaryTest.kt | New adversarial tests proving secrets don’t leak across persistence boundaries and observer behavior is fail-open + cancellation-correct. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/WorkflowTest.kt | Updates assertions to fixed persistence conflict messages + cause-free failures. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/WorkflowRecoveryContractTest.kt | Updates internal codec expectations to use corruption carrier exceptions (public fixed text proven at store boundary). |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/WorkflowLeaseStoreTest.kt | Updates lease conflict assertions to fixed messages + cause-free failures. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/WorkflowCheckpointStoreTest.kt | Updates checkpoint conflict assertions to fixed messages + cause-free failures. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/StepAttemptRecordStoreContractTest.kt | Updates step-attempt persistence failure expectations to safe fixed-text failures and internal corruption carrier usage. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/JdbcWorkflowPersistenceCancellationContractTest.kt | Updates lease-conflict assertion to fixed message + cause-free failure. |
| tramai-orchestration/src/test/kotlin/dev/tramai/orchestration/BinaryCompatibilityFixtureTest.kt | Extends binary fixture marker coverage to validate additional ABI-preservation cases. |
| tramai-orchestration/src/test/resources/binary-compat/BinaryCompatFixture.kt | Extends fixture code to exercise additional persistence exception/store constructors. |
| tramai-orchestration/src/test/resources/binary-compat/README.md | Documents updated fixture scope and exercised constructors. |
| tramai-orchestration/api/tramai-orchestration.api | API dump updates for newly-added persistence failure types/observers and additive constructors/getters. |
| docs/concepts/safe-error-boundaries.md | Documents persistence safe-failure boundary as implemented and completes Epic 1.2 scope. |
| docs/guides/orchestration-persistence.md | Adds usage guidance for PersistenceFailureDiagnosticObserver and fixed public failure behavior. |
| docs/ROADMAP-0.6.0.md | Marks Epic 1.2 as complete and records persistence boundary completion. |
| docs/releases/0.6.0-characterization-matrix.md | Updates characterization entries to include persistence safe-failure boundary coverage. |
| CHANGELOG.md | Adds release note entry describing safe persistence failure boundaries and Epic 1.2 completion. |
Suppressed comments (1)
tramai-orchestration/src/main/kotlin/dev/tramai/orchestration/WorkflowPersistence.kt:145
clearRecoverywraps the initialloadin apersistenceBoundarylabeled asPersistenceOperation.SAVE. If a custom store’sloadthrows a raw exception, it will be classified/emitted as a write failure instead of a read failure. Consider splitting into a LOAD boundary (for the read) followed by a SAVE boundary (for the write).
return persistenceBoundary(
PersistenceResourceKind.CHECKPOINT,
PersistenceOperation.SAVE,
checkpointDiagnosticObserver(this),
) {
val current = load(workflowName, workflowId)
?: throw safePersistenceFailure(
PersistenceResourceKind.CHECKPOINT,
PersistenceOperation.SAVE,
PersistenceFailureCode.CONFLICT,
)
save(
checkpoint = current.copy(recoveryState = WorkflowRecoveryState.Normal),
expectedRevision = expectedRevision,
)
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ize attempt summaries - persistenceBoundary now ends with currentCoroutineContext().ensureActive() so a parent cancellation arriving while the block completes normally still wins over a normal return (the #223/#224 post-callback rule applied to the boundary itself). Fixes the CI-flaky genuine-parent-cancellation boundary test: release.complete(Unit) could race the parent cancel, making the block return normally and the test observe null instead of CancellationException. - ExecutionTracker.failAttempt sanitizes persistence-family failures for both the ordinary observer and the persisted outputSummary, while user step-execution errors keep their real message. Adds Throwable.isPersistenceFamilyFailure() and a boundary test locking the split (secret stripped from persistence summaries, user messages preserved).
…d cleanup leaks Round-2 review fixes (safe persistence boundaries): - persistenceBoundary no longer trusts throwable identity: trusted failures are reconstructed as fresh cause-free/suppressed-free fixed-text instances of the same semantic class (JDBC cleanup mutates the primary after construction via addSuppressed). Contaminated trusted throwables are delivered to the observer before reconstruction; clean trusted failures emit no event. - Cancellation with raw JDBC children (Statement.cancel, rollback, auto-commit restore, close) is sanitized: raw graph goes to the observer, the caller-visible CancellationException is a fresh fixed-text instance with only a SanitizedCleanupDiagnosticException marker. Genuine framework cancellation passes through unchanged and emits no diagnostic. Fixed CE message text. - FileWorkflowLeaseStore.release/deleteCheckpointIfLeaseOwner resolve the path inside persistenceBoundary so a throwing WorkflowCheckpointPathStrategy cannot escape the boundary. - JdbcWorkflowLeaseStore fenced save/delete route checkpoint DML failures to the checkpoint store's observer with resourceKind=CHECKPOINT via a CheckpointDmlFailure marker; lease-row/fence failures stay on the lease channel (exactly one event per failing phase). - requireRecovery/clearRecovery split into LOAD- and SAVE-phase boundaries so a load failure is READ_FAILED, not mislabeled WRITE_FAILED. - defaultPersistenceFailureCode drops the class-name string heuristic; CLAIM/RENEW -> WRITE_FAILED, RELEASE -> DELETE_FAILED. - safeWorkerObservableFailure reconstructs trusted failures instead of passing the throwable identity to observers. - PersistenceSafeFailureBoundaryTest extended to 25 adversarial scenarios: cancellation suppressed-graph sanitization, trusted contamination, throwing path strategy, phase-aware classification, checkpoint observer routing, and cancellation precedence over trusted failures.
…diagnostics, hoisted preconditions Round-3 review fixes (safe persistence boundaries): - Cancellation sanitization no longer routes through the diagnostic-delivery helper: delivery ends with ensureActive(), which throws under genuine parent cancellation before the sanitized CE could escape. A contaminated CE now becomes a fresh fixed-text CE with only a SanitizedCleanupDiagnosticException marker, thrown immediately; no observer event on the genuine-cancellation path (documented contract). Regression added with a genuinely inactive coroutine context. - Lease compound operations (CLAIM/RENEW/RELEASE) classify read-phase failures as READ_FAILED via an internal LeaseReadPhaseFailure marker raised by the JDBC and file leaf read helpers; the outer operation context is preserved. - Corrupt checkpoint data inside fenced JDBC DML stays CORRUPTED_DATA (WorkflowCheckpointCorruptionException) instead of a generic WRITE/DELETE failure; the generic boundary fallback also classifies internal corrupt carriers so no call site depends on passing a classify lambda. - Caller/framework preconditions hoisted outside persistence boundaries: JDBC fenced save (store-type and DataSource checks), InMemory worker heartbeat (unknown worker) and stale-worker threshold validation now surface as IllegalArgumentException, matching the delete fence and pre-#225 behavior. - New public PersistenceCorruptionDetail interface exposes the corrupt raw payload to diagnostic observers (additive ABI; internal corrupt carriers implement it); caller-visible exceptions stay redacted. - PersistenceSafeFailureBoundaryTest extended to 33 adversarial scenarios: sanitized-cancellation-under-genuine-cancellation, corrupt fenced DML, precondition surfacing, external observer corrupt-payload access, and lease read-phase classification.
…heartbeat, delete-phase classification Round-4 review fixes (safe persistence boundaries): - Cancellation contamination detection is now recursive and cycle-safe: hasUnsafeCancellationDetail walks the full cause+suppressed graph, so a raw SQLException nested under an inner CancellationException is caught and sanitized (previously only direct children were inspected). - PersistenceCleanupDiagnosticException replaces the process-cleanup marker in sanitized cancellation exceptions (no cross-domain 'Process cleanup' text on a cancelled JDBC persistence operation). - updateHeartbeat check-then-update restored as ONE atomic monitor operation (round-3 hoisting had introduced an unsynchronized LinkedHashMap read and a check-then-act race with unregisterWorker); clockMillis stays inside the boundary so user-clock failures are sanitized, unknown-worker IllegalArgumentException surfaces unchanged. - Lease DELETE phases (expired-file cleanup during LOAD/RENEW) classified DELETE_FAILED via LeaseDeletePhaseFailure marker, mirroring the read-phase fix; outer operation context preserved. - PersistenceCorruptionDetail KDoc documents that fenced-JDBC corruption arrives via the cause chain of CheckpointDmlFailure. - Recursion guard switched from IdentityHashMap to per-call ArrayList with identity comparison: the IdentityHashMap default-arg matched the global-state scanner's broad HashMap pattern, producing a false NEW_GLOBAL_STATE finding attributed to an unrelated file. - PersistenceSafeFailureBoundaryTest extended to 40 adversarial scenarios: nested-cancellation sanitization, delete/read phase classification, atomic heartbeat vs unregister, precondition surfacing.
agy round-4 P3: the atomicity test commented 'heartbeats and unregisters' but only re-registered. Odd iterations now unregisterWorker before the heartbeat, exercising the real check-then-act race; observed outcomes stay success or IllegalArgumentException, never NoSuchElementException.
Summary
Finishes Epic 1.2 — Safe Error Boundaries (
docs/ROADMAP-0.6.0.md): establish safe persistence failureboundaries so filesystem paths, SQL text, JDBC driver messages, persisted state, metadata, identifiers, and
arbitrary exception messages never cross from persistence internals into caller-visible exceptions, ordinary
observers, logs, audit/evidence, or telemetry. The original failure stays available to an explicitly configured
diagnostic sink; cancellation stays cancellation; existing concurrency/recovery semantics and the exception ABI
are unchanged.
What changed
PersistenceFailures.kt):PersistenceFailureCode(READ_FAILED, WRITE_FAILED,DELETE_FAILED, LIST_FAILED, CONFLICT, CORRUPTED_DATA),
PersistenceResourceKind(CHECKPOINT, LEASE,STEP_ATTEMPT, WORKER_REGISTRY),
PersistenceOperation,PersistenceFailureDiagnosticObserver+PersistenceFailureDiagnosticEvent,NoOpPersistenceFailureDiagnosticObserver,WorkflowPersistenceFailureException,safePersistenceFailure()(fixed enum-derived text),persistenceBoundary()(cancellation-first → trustedreconstruction → observer delivery → safe throw, post-block
ensureActive()), anddeliverPersistenceFailurewith post-observer
ensureActive().cause-free/suppressed-free fixed-text instances of the same semantic class (JDBC cleanup mutates the primary
after construction via
addSuppressed). Contaminated trusted throwables are delivered to the observer first.CancellationExceptionwhose graph (cause + suppressed, any depth,cycle-safe) contains a raw JDBC child becomes a fresh fixed-text CE with only a
PersistenceCleanupDiagnosticExceptionmarker, thrown immediately — no observer event on thegenuine-cancellation path (delivery's postcondition
ensureActive()must not run under parent cancellation).Framework
JobCancellationExceptionchains pass through untouched.stores, and worker registry. Internal corrupt carriers (
CorruptCheckpointException/CorruptStepAttemptException)hold the raw payload; public
WorkflowCheckpointCorruptionException/StepAttemptRecordCorruptionExceptionsurface only at store boundaries.
operation supplies the operation context — lease read phases (CLAIM/RENEW/RELEASE) classify READ_FAILED,
lease delete phases (expired-file cleanup during LOAD/RENEW) classify DELETE_FAILED, fenced JDBC checkpoint
DML routes to the checkpoint observer with
resourceKind=CHECKPOINT, and corrupt data inside fenced DML staysCORRUPTED_DATA (
WorkflowCheckpointCorruptionException).surface as
IllegalArgumentException, not persistence failures;updateHeartbeatcheck-then-update remainsone atomic monitor operation so unknown-worker errors surface deterministically.
TramaiWorkerobserver callbacks receivesafeWorkerObservableFailure(...);LoggingTramaiWorkerObserverlogs exception class names only;failAttemptpersists a sanitized summary forpersistence-family failures while keeping real user step-error messages.
PersistenceCorruptionDetailinterface lets diagnostic observers read the corrupt raw payload(additive ABI; caller-visible exceptions stay redacted).
descriptors byte-identical);
BinaryCompatibilityFixtureTestrebuilt against the v0.5.0 tag scope.Verification
./gradlew test --rerun-tasksgreen across modules (orchestration: 400+ tests incl. 40-testPersistenceSafeFailureBoundaryTestadversarial leak suite).verifyCancellationSafetyvs base PASSED (no new critical/high findings).verifyPr -PchangeClass=public-apiPASSED (4 pre-existing warnings onTramaiWorker.kt/Tramai.kt, present on master).apiCheckPASSED after apiDump.Fix rounds (review findings addressed)
f2340ffd→2367d0fc5daf9c9bd804a443PersistenceCorruptionDetail; Copilot thread resolved0378fda1Scope notes
.hermes/plans/*.mdare working notes and are intentionally not committed.requireRecoverythread resolved.